Skip to content

fix(#304): implement ReferenceConstraint residuals and verify post-solve#305

Merged
prjemian merged 1 commit into
mainfrom
304-reference-constraint-cleanup
Jun 24, 2026
Merged

fix(#304): implement ReferenceConstraint residuals and verify post-solve#305
prjemian merged 1 commit into
mainfrom
304-reference-constraint-cleanup

Conversation

@prjemian

Copy link
Copy Markdown
Collaborator

Reframing the issue

Issue #304 reported that ReferenceConstraint.evaluate() / .is_satisfied()
were stubs raising NotImplementedError, concluding the reference-constraint
solvers were never finished. Investigation showed the premise is inverted:

  • The forward solvers for every reference-constraint mode
    (incidence, emergence, specular, psi, omega) already exist in
    forward.py (_solve_surface, _solve_psi_mode, _solve_omega_mode,
    _solve_free_detectors, _solve_qaz_mode). The dispatcher routes those
    modes to the dedicated solvers and never calls the stubbed methods.
  • ConstraintSet.is_implemented() / ReferenceConstraint.is_implemented()
    already return True once the reference vector (azimuth / surface_normal)
    is set.
  • Every affected mode (psic, fourcv, sixc, zaxis, s2d2, kappa*)
    solves end-to-end through forward() once its reference vector is set —
    verified across all of them.

The real defect was therefore dead code + stale references, not a missing
solver. The two stub methods were reachable only from two tests that asserted
they raise, and the comments pointed at the long-closed "Issue J" (#157).

What this PR does

  • mode.py: evaluate() returns real pseudo-angle residuals via reference.py
    (incidence/emergence/specular/psi/naz/omega); is_satisfied()
    delegates to it; the stale "Issue J" comments are removed.
  • forward.py: reference constraints are wired into the post-solve verification
    loop (_validate_solutions), so reference modes get the same residual sanity
    check as bisect/sample constraints. psi is intentionally skipped there
    — it is enforced upstream as a validation filter in _solve_psi_mode, and its
    motor-frame residual is subject to ±360° azimuthal wraparound, so a naive
    per-solution check is unreliable. Unset reference vectors are guarded.
  • Tests: the two NotImplementedError stub tests are replaced with real residual
    tests; the is_implemented parametrization is corrected (psi is True when
    azimuth is set; naz is False). A new cross-module regression file
    tests/test_regression_issue_304.py confirms every affected mode solves
    without spurious ConstraintViolation and that the loop rejects a bad
    reference solution.

Known remaining gap (out of scope, not a regression)

naz has no forward solverforward.py has no _solve_naz_mode, and
ReferenceConstraint.is_implemented() deliberately returns False for it.
evaluate() will compute a naz residual (used for verification only), but no
demo geometry's YAML declares a naz mode, so nothing is broken. Per the
pre-1.0 no-volunteered-follow-up policy, this is noted here rather than filed as
a new issue.

QC

  • Full suite: 2644 passed, 2 skipped, 100% coverage.
  • pytest -m slow_benchmark: 3 passed (hot path: forward.py / mode.py).
  • US-English grep on changed files: clean.
  • pre-commit run: all hooks pass.

Contributed by: OpenCode (argo/claudeopus48)

ReferenceConstraint.evaluate()/.is_satisfied() were stubs raising
NotImplementedError, with comments pointing at the long-closed "Issue J"
(#157).  The forward solvers for every reference-constraint mode
(incidence/emergence/specular/psi/omega) already existed; the dispatcher
routed those modes to dedicated _solve_* functions and never called the
stubs.

- mode.py: evaluate() now returns real pseudo-angle residuals via
  reference.py (incidence/emergence/specular/psi/naz/omega);
  is_satisfied() delegates to it; remove stale "Issue J" comments.
- forward.py: wire reference constraints into the post-solve verification
  loop (skip "psi", enforced upstream as a validation filter and subject
  to +/-360 deg azimuthal wraparound; guard unset reference vectors);
  fix the stale "not yet implemented" comment.
- tests: replace the two NotImplementedError stub tests with real
  residual tests; fix is_implemented parametrization (psi True with
  azimuth, naz False); add cross-module regression test_regression_issue_304.

naz still has no forward solver (is_implemented stays False); no demo
geometry uses a naz mode, so nothing is broken.

Contributed by: OpenCode (argo/claudeopus48)
@prjemian prjemian added this to the v0.11.2 patches milestone Jun 24, 2026
@prjemian prjemian merged commit 3992835 into main Jun 24, 2026
7 checks passed
@prjemian prjemian deleted the 304-reference-constraint-cleanup branch June 24, 2026 19:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ReferenceConstraint solvers still stubbed in v0.11.2: psi/incidence/emergence/specular modes raise NotImplementedError

1 participant